Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

make HEAD/GET method rewrite configurable #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

make HEAD/GET method rewrite configurable #20

wants to merge 1 commit into from

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Feb 8, 2020

closes #15

@azjezz azjezz requested a review from jjergus February 8, 2020 20:21
@jjergus jjergus requested a review from fredemmott February 10, 2020 16:02
@jjergus
Copy link
Contributor

jjergus commented Feb 10, 2020

Seems reasonable but Fred has stronger opinions about this so I'll leave it to him. Some potential minor concerns:

  • if we later decided on a different solution and removed the flag, it would break backwards compatibility so we should be reasonably sure we want to stick to this solution
  • boolean flags can be confusing (as part of a public API, that is -- they're fine in private methods etc.), for someone reading the code it's impossible to know what new SomeClass(false) means, consider replacing that with either
    • a method call: (new SomeClass())->disableHeadRewrite()->...
    • private constructor + pair of static "factory" methods: SomeClass::withHeadRewrite()->... and SomeClass::withoutHeadRewrite()->...

@fredemmott
Copy link
Contributor

I'm fine with the intent of this diff, but not the bool argument; I'm fine with a shape argument, or the instance methods.

The problem with static factory methods is it's not scalable if we want to add other options in the future.

@azjezz azjezz force-pushed the patch-2 branch 2 times, most recently from 031136c to da5fc0b Compare February 10, 2020 19:58
@azjezz
Copy link
Contributor Author

azjezz commented Feb 10, 2020

updated to use options shape 👍

@azjezz azjezz changed the title make head -> get rewrite configurable make HEAD/GET method rewrite configurable Feb 10, 2020
fredemmott
fredemmott reviewed Feb 13, 2020
View reviewed changes
src/router/BaseRouter.php
@@ -14,6 +14,10 @@
use function Facebook\AutoloadMap\Generated\is_dev;

abstract class BaseRouter<+TResponder> {
public function __construct(
private BaseRouterOptions $options = shape('use_get_responder_for_head' => true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps ?BaseRouterOptions, and make all the fields optional?

This will be needed when a second option is added - it doesn't really matter now, but doing it would make it clearer what the 'right way' is to add one.

This would also need something like:

$this->options = shape(
  'use_get_responder_for_head' => $options['use_get_responder_for_head'] ?? true
);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fredemmott fredemmott fredemmott left review comments

@jjergus jjergus Awaiting requested review from jjergus

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove HEAD/GET rewrite
4 participants
@azjezz @jjergus @fredemmott @facebook-github-bot